-
Notifications
You must be signed in to change notification settings - Fork 987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refs #37440 - Readd host registration repo parameters with deprecatio… #10187
Refs #37440 - Readd host registration repo parameters with deprecatio… #10187
Conversation
@stejskalleos Fix for re-adding the host registration parameters. Theoretically, you can add both parameters now, repo and repo_data. repo_data would overwrite repo parameters if available. |
16f57b8
to
87e5376
Compare
@@ -18,6 +18,9 @@ class RegistrationCommandsController < V2::BaseController | |||
param :insecure, :bool, desc: N_("Enable insecure argument for the initial curl") | |||
param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`") | |||
param :update_packages, :bool, desc: N_("Update all packages on the host") | |||
param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run bin/hammer host-registration generate-command --help
, I don't see any information about the deprecation.
@ofedoren What does the deprecated
do? I thought it would mark the fields in hammer --help
output as deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, hammer doesn't do that. Mostly we need to do that manually, e.g. in the command definition itself we need to add build_options without: repo, :repo_gpg_key_url
. And then add them as options manually with the deprecation warning like in https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/command_extensions/compute_resource_subcommand.rb#L5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how is the workflow? Shall I open a hammer PR in parallel or do we get this one in, first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how do we proceed here? I think it should go in asap because #10162 has a breaking change in the API (that is reverted in this PR) and is part of 3.11. So the fix should also go into 3.11
@@ -23,13 +23,20 @@ class RegistrationController < V2::BaseController | |||
param :setup_remote_execution, :bool, desc: N_("Set 'host_registration_remote_execution' parameter for the host. If it is set to true, SSH keys will be installed on the host") | |||
param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`") | |||
param :update_packages, :bool, desc: N_("Update all packages on the host") | |||
param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run the hammer command
bundle exec bin/hammer host-registration generate-command --repo "https://repo.com"
I don't see any deprecation warning in Foreman / Hammer log.
@nadjaheitmann does it work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it in the console output of my Forklift so I assume it would appear in production log. Not sure how helpful that is, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I can see it if I run the command, not for the help output.
If you look at the apidoc, the parameters appear as deprecated, though.
if params["repo"].present? | ||
repo_data = {} | ||
repo_data[params['repo']] = params['repo_gpg_key_url'] || '' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to be the same, is this intentional? https://github.com/theforeman/foreman/pull/10162/files#diff-05645e27e9ae6b798390ab2c548df6516a13db34d245e54eee0690c965bffbf4R21-R24
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is supposed to create a hash where the key
is the repository URL and the value
is the corresponding GPG key. In this case, if you have the old parameters given, it just creates a hash with a single key value pair that is evaluated at a later point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know what you mean. I messed up the params names but should be sorted out now.
@@ -18,6 +18,9 @@ class RegistrationCommandsController < V2::BaseController | |||
param :insecure, :bool, desc: N_("Enable insecure argument for the initial curl") | |||
param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`") | |||
param :update_packages, :bool, desc: N_("Update all packages on the host") | |||
param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, hammer doesn't do that. Mostly we need to do that manually, e.g. in the command definition itself we need to add build_options without: repo, :repo_gpg_key_url
. And then add them as options manually with the deprecation warning like in https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/command_extensions/compute_resource_subcommand.rb#L5
a82fa16
to
ae67cf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the description for parameters as well, the hammer part can be done later on and we don't have to block it on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did quick test and it doesn't work for me as expected:
I added two repositories:
http://rpm111.example.com, key: https://test11.com
http://rpm222.example.com, https://test22.com
The output of the template is:
[foreman_register1]
name=foreman_register1
baseurl=''
enabled=1
type=rpm-md
EOF
echo gpgcheck=1 >> /tmp/foreman_registration.repo
echo gpgkey=https://test22.com >> /tmp/foreman_registration.repo
- I don't see the second repo set anywhere
- There is no
baseurl
- The gpgkey for repo2 is used for repo1
ae67cf5
to
a68f861
Compare
There was a small bug which affected the UI. Can you retest, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍏 LGTM
- UI Form is working
- API accepts new parameters correctly
- Old parameters work as well, but logs the deprecation message
- Repos are set correctly, and the CentOS stream 9 machine has been registered successfully
Thanks @nadjaheitmann |
Thanks for testing @stejskalleos and @Manisha15 |
…n warning